RSPEED-2326: feat(rlsapi): integrate Splunk telemetry into v1 /infer endpoint #1050
+648
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds Splunk HEC telemetry emission: the inference endpoint now accepts FastAPI Changes
Sequence DiagramsequenceDiagram
participant Client
participant InferEndpoint as "infer_endpoint\n(Request, BackgroundTasks)"
participant RHContext as "RH Identity\nContext"
participant Processor as "Inference\nProcessing"
participant BackgroundTasks as "BackgroundTasks"
participant SplunkHEC as "Splunk HEC"
Client->>InferEndpoint: POST /rlsapi/v1/infer
InferEndpoint->>RHContext: extract org_id, system_id
RHContext-->>InferEndpoint: context or AUTH_DISABLED
InferEndpoint->>InferEndpoint: start inference timer
InferEndpoint->>Processor: run inference
alt Success
Processor-->>InferEndpoint: response_text
InferEndpoint->>InferEndpoint: compute inference_time
InferEndpoint->>BackgroundTasks: add_task(_queue_splunk_event, success payload + RH context)
else API/RateLimit/Status Error
Processor-->>InferEndpoint: raises error
InferEndpoint->>InferEndpoint: compute inference_time
InferEndpoint->>BackgroundTasks: add_task(_queue_splunk_event, error payload + RH context)
end
InferEndpoint-->>Client: response or HTTP error
BackgroundTasks->>SplunkHEC: async send event
SplunkHEC-->>BackgroundTasks: acknowledgement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
major
force-pushed
the
rspeed-2326-splunk-endpoint
branch
from
0ae1460 to
40b19fa
Compare
…endpoint - Add _get_rh_identity_context() to extract org_id/system_id from request.state - Add _queue_splunk_event() to build and queue telemetry events via BackgroundTasks - Add timing measurement around inference calls - Queue infer_with_llm events on success, infer_error on failure - Add unit tests for RH Identity context extraction and Splunk integration - Update integration tests for new endpoint signature - Add user-facing docs (docs/splunk.md) and developer docs (src/observability/README.md) Signed-off-by: Major Hayden <major@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/splunk.md`:
- Around line 139-142: The fenced log snippet in docs/splunk.md is missing a
language tag which triggers markdownlint MD040; update the code fence
surrounding the log line "Splunk HEC request failed with status 403: Invalid
token" from ``` to ```text so the block is explicitly marked as plain text and
renders/lints correctly.
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 262-317: The except branches currently call
_queue_splunk_event(...) then raise HTTPException(...), which drops FastAPI
background tasks and loses telemetry; instead, after recording inference_time,
metrics, logging, and calling _queue_splunk_event(...), construct the
appropriate response model (ServiceUnavailableResponse, QuotaExceededResponse,
or InternalServerErrorResponse.generic()), convert it to a dict via
model_dump(), and return a fastapi.responses.JSONResponse with that body, the
correct status_code, and background=background_tasks so the queued telemetry
runs; update the handlers for APIConnectionError, RateLimitError, and
APIStatusError (the except blocks around retrieve_simple_response) to return
JSONResponse(...) rather than raising HTTPException.
In `@tests/integration/endpoints/test_rlsapi_v1_integration.py`:
- Around line 40-46: In _create_mock_request replace the deletion of the
non-existent attribute (del mock_request.state.rh_identity_data) with explicitly
setting mock_request.state.rh_identity_data = None so it won't raise
AttributeError; update the mock setup in the _create_mock_request function to
assign None instead of deleting the attribute to reflect absent identity data.
In `@tests/unit/app/endpoints/test_rlsapi_v1.py`:
- Around line 40-50: The helper _create_mock_request should avoid
unconditionally deleting mock_request.state.rh_identity_data because del will
raise AttributeError if the attribute doesn't exist; instead, check for
existence (e.g., hasattr/getattr) and only delete or set to None when present.
Update the _create_mock_request implementation to conditionally remove or clear
mock_request.state.rh_identity_data using a safe existence check so tests won’t
break when the attribute was never set.
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
182-196: Bring helper docstrings up to Google style.
_get_cla_versionand_queue_splunk_eventneed full Args/Returns sections to match module conventions.As per coding guidelines, please keep Google-style docstrings for all functions.✍️ Suggested docstring shape
-def _get_cla_version(request: Request) -> str: - """Extract CLA version from User-Agent header.""" +def _get_cla_version(request: Request) -> str: + """Extract CLA version from the User-Agent header. + + Args: + request: The FastAPI request object. + + Returns: + The User-Agent header value or an empty string. + """ @@ def _queue_splunk_event( @@ ) -> None: - """Build and queue a Splunk telemetry event for background sending.""" + """Build and queue a Splunk telemetry event for background sending. + + Args: + background_tasks: FastAPI background tasks queue. + infer_request: Inference request payload. + request: FastAPI request for headers/state. + request_id: Unique request identifier. + response_text: LLM response or error message. + inference_time: Total inference duration in seconds. + sourcetype: Splunk sourcetype (e.g., infer_with_llm). + """
Comment on lines
+139
to
+142
| 4. Check service logs for warning messages: | ||
| ``` | ||
| Splunk HEC request failed with status 403: Invalid token | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced log snippet.
This avoids markdownlint MD040 and improves rendering.
🔧 Suggested fix
- ```
- Splunk HEC request failed with status 403: Invalid token
- ```
+ ```text
+ Splunk HEC request failed with status 403: Invalid token
+ ```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/splunk.md` around lines 139 - 142, The fenced log snippet in
docs/splunk.md is missing a language tag which triggers markdownlint MD040;
update the code fence surrounding the log line "Splunk HEC request failed with
status 403: Invalid token" from ``` to ```text so the block is explicitly marked
as plain text and renders/lints correctly.
Comment on lines
+262
to
317
| start_time = time.monotonic() | ||
| try: | ||
| response_text = await retrieve_simple_response(input_source, instructions) | ||
| inference_time = time.monotonic() - start_time | ||
| except APIConnectionError as e: | ||
| inference_time = time.monotonic() - start_time | ||
| metrics.llm_calls_failures_total.inc() | ||
| logger.error( | ||
| "Unable to connect to Llama Stack for request %s: %s", request_id, e | ||
| ) | ||
| _queue_splunk_event( | ||
| background_tasks, | ||
| infer_request, | ||
| request, | ||
| request_id, | ||
| str(e), | ||
| inference_time, | ||
| "infer_error", | ||
| ) | ||
| response = ServiceUnavailableResponse( | ||
| backend_name="Llama Stack", | ||
| cause=str(e), | ||
| ) | ||
| raise HTTPException(**response.model_dump()) from e | ||
| except RateLimitError as e: | ||
| inference_time = time.monotonic() - start_time | ||
| metrics.llm_calls_failures_total.inc() | ||
| logger.error("Rate limit exceeded for request %s: %s", request_id, e) | ||
| _queue_splunk_event( | ||
| background_tasks, | ||
| infer_request, | ||
| request, | ||
| request_id, | ||
| str(e), | ||
| inference_time, | ||
| "infer_error", | ||
| ) | ||
| response = QuotaExceededResponse( | ||
| response="The quota has been exceeded", cause=str(e) | ||
| ) | ||
| raise HTTPException(**response.model_dump()) from e | ||
| except APIStatusError as e: | ||
| inference_time = time.monotonic() - start_time | ||
| metrics.llm_calls_failures_total.inc() | ||
| logger.exception("API error for request %s: %s", request_id, e) | ||
| _queue_splunk_event( | ||
| background_tasks, | ||
| infer_request, | ||
| request, | ||
| request_id, | ||
| str(e), | ||
| inference_time, | ||
| "infer_error", | ||
| ) | ||
| response = InternalServerErrorResponse.generic() | ||
| raise HTTPException(**response.model_dump()) from e |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastAPI BackgroundTasks executed when raising HTTPException
💡 Result:
BackgroundTasks in FastAPI (Starlette) only run if they are attached to the Response that gets sent—they execute after the response is sent. If you raise HTTPException, FastAPI aborts the normal return path and builds a new error response, so the tasks you added in the endpoint won’t be attached to the response and won’t run. [1], [2]
Options
A) Return an error response instead of raising, with a background task attached
from fastapi import FastAPI
from starlette.responses import JSONResponse
from starlette.background import BackgroundTask
app = FastAPI()
def log_failure(msg: str):
...
`@app.get`("/x")
def x():
return JSONResponse(
{"detail": "bad"},
status_code=400,
background=BackgroundTask(log_failure, "bad"),
)(Background tasks must be attached to the response.) [2]
B) If you must raise HTTPException, attach tasks in a custom exception handler
from fastapi import FastAPI, BackgroundTasks
from starlette.exceptions import HTTPException as StarletteHTTPException
from starlette.responses import PlainTextResponse
app = FastAPI()
def log_failure(msg: str):
...
`@app.exception_handler`(StarletteHTTPException)
async def http_exc_handler(request, exc):
tasks = BackgroundTasks()
tasks.add_task(log_failure, str(exc.detail))
return PlainTextResponse(str(exc.detail), status_code=exc.status_code, background=tasks)(Workaround: build the error response yourself and attach tasks there.) [3], [4]
C) If you need “always run”, use try/finally (not BackgroundTasks)
Because BackgroundTasks is specifically “run after a response is sent,” it’s not the right tool for cleanup that must happen even when you abort early. [1]
Sources: FastAPI BackgroundTasks docs [1]; Starlette Background Tasks docs [2]; Stack Overflow workaround using custom exception handler [3]; Starlette exceptions/response behavior [4].
Background tasks will not execute when raising HTTPException—adjust to preserve telemetry.
FastAPI background tasks only execute if attached to the returned Response. Raising HTTPException aborts the normal response path, causing the _queue_splunk_event() background tasks to be dropped and infer_error telemetry to be lost. Return a JSONResponse with the background parameter instead, or implement a custom exception handler to attach tasks to the error response.
Fix pattern (apply to each error branch)
+from fastapi.responses import JSONResponse
@@
except APIConnectionError as e:
inference_time = time.monotonic() - start_time
metrics.llm_calls_failures_total.inc()
logger.error(
"Unable to connect to Llama Stack for request %s: %s", request_id, e
)
_queue_splunk_event(
background_tasks,
infer_request,
request,
request_id,
str(e),
inference_time,
"infer_error",
)
response = ServiceUnavailableResponse(
backend_name="Llama Stack",
cause=str(e),
)
- raise HTTPException(**response.model_dump()) from e
+ error_payload = response.model_dump()
+ return JSONResponse(
+ status_code=error_payload["status_code"],
+ content=error_payload["detail"],
+ background=background_tasks,
+ )🤖 Prompt for AI Agents
In `@src/app/endpoints/rlsapi_v1.py` around lines 262 - 317, The except branches
currently call _queue_splunk_event(...) then raise HTTPException(...), which
drops FastAPI background tasks and loses telemetry; instead, after recording
inference_time, metrics, logging, and calling _queue_splunk_event(...),
construct the appropriate response model (ServiceUnavailableResponse,
QuotaExceededResponse, or InternalServerErrorResponse.generic()), convert it to
a dict via model_dump(), and return a fastapi.responses.JSONResponse with that
body, the correct status_code, and background=background_tasks so the queued
telemetry runs; update the handlers for APIConnectionError, RateLimitError, and
APIStatusError (the except blocks around retrieve_simple_response) to return
JSONResponse(...) rather than raising HTTPException.
Comment on lines
+40
to
+46
| def _create_mock_request(mocker: MockerFixture) -> Any: | ||
| """Create a mock FastAPI Request with minimal state.""" | ||
| mock_request = mocker.Mock() | ||
| mock_request.state = mocker.Mock() | ||
| mock_request.headers = {"User-Agent": "CLA/0.4.0"} | ||
| del mock_request.state.rh_identity_data | ||
| return mock_request |
Contributor
There was a problem hiding this comment.
Same mock deletion issue here.
Deleting an attribute that was never set can raise AttributeError. Use an explicit None instead.
🔧 Suggested fix
- del mock_request.state.rh_identity_data
+ mock_request.state.rh_identity_data = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _create_mock_request(mocker: MockerFixture) -> Any: | |
| """Create a mock FastAPI Request with minimal state.""" | |
| mock_request = mocker.Mock() | |
| mock_request.state = mocker.Mock() | |
| mock_request.headers = {"User-Agent": "CLA/0.4.0"} | |
| del mock_request.state.rh_identity_data | |
| return mock_request | |
| def _create_mock_request(mocker: MockerFixture) -> Any: | |
| """Create a mock FastAPI Request with minimal state.""" | |
| mock_request = mocker.Mock() | |
| mock_request.state = mocker.Mock() | |
| mock_request.headers = {"User-Agent": "CLA/0.4.0"} | |
| mock_request.state.rh_identity_data = None | |
| return mock_request |
🤖 Prompt for AI Agents
In `@tests/integration/endpoints/test_rlsapi_v1_integration.py` around lines 40 -
46, In _create_mock_request replace the deletion of the non-existent attribute
(del mock_request.state.rh_identity_data) with explicitly setting
mock_request.state.rh_identity_data = None so it won't raise AttributeError;
update the mock setup in the _create_mock_request function to assign None
instead of deleting the attribute to reflect absent identity data.
Comment on lines
+40
to
+50
| def _create_mock_request(mocker: MockerFixture, rh_identity: Any = None) -> Any: | ||
| """Create a mock FastAPI Request with optional RH Identity data.""" | ||
| mock_request = mocker.Mock() | ||
| mock_request.state = mocker.Mock() | ||
| mock_request.headers = {"User-Agent": "CLA/0.4.0"} | ||
|
|
||
| if rh_identity is not None: | ||
| mock_request.state.rh_identity_data = rh_identity | ||
| else: | ||
| del mock_request.state.rh_identity_data | ||
|
|
Contributor
There was a problem hiding this comment.
Mock attribute deletion can raise AttributeError.
del mock_request.state.rh_identity_data will fail when the attribute hasn’t been created yet. This makes the helper brittle and can break multiple tests.
🔧 Suggested fix
- else:
- del mock_request.state.rh_identity_data
+ else:
+ mock_request.state.rh_identity_data = None🤖 Prompt for AI Agents
In `@tests/unit/app/endpoints/test_rlsapi_v1.py` around lines 40 - 50, The helper
_create_mock_request should avoid unconditionally deleting
mock_request.state.rh_identity_data because del will raise AttributeError if the
attribute doesn't exist; instead, check for existence (e.g., hasattr/getattr)
and only delete or set to None when present. Update the _create_mock_request
implementation to conditionally remove or clear
mock_request.state.rh_identity_data using a safe existence check so tests won’t
break when the attribute was never set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Integrates Splunk HEC telemetry into the rlsapi v1
/inferendpoint. This is the final PR in the Splunk integration series (building on #1031 and #1032).Changes:
_get_rh_identity_context()helper to extract org_id/system_id from request.state_queue_splunk_event()to build and queue telemetry events via FastAPI BackgroundTasksinfer_with_llmevents on success,infer_errorevents on failuredocs/splunk.md)src/observability/README.md)Events are sent asynchronously and never block or affect the main request flow.
Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run pytest tests/unit/app/endpoints/test_rlsapi_v1.py -v(24 tests)uv run pytest tests/integration/endpoints/test_rlsapi_v1_integration.py -v(11 tests)uv run pytest tests/unit/observability/ -v(11 tests)New tests verify:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.